Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat!: no-unknown-at-rules -> no-invalid-at-rules #12

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

feat!: no-unknown-at-rules -> no-invalid-at-rules #12

wants to merge 10 commits into from

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Nov 19, 2024

Prerequisites checklist

What is the purpose of this pull request?

Renamed no-unknown-at-rules to no-invalid-at-rules to cover more cases.

What changes did you make? (Give an overview)

  • Renamed source files from no-unknown-at-rules.* to no-invalid-at-rules.*
  • Added checks for invalid preludes, unknown descriptors, and invalid descriptor values.
  • Added more tests
  • Updated docs

Related Issues

Is there anything you'd like reviewers to focus on?

There are type errors that will be resolved once #11 is merged and I can rebase on top of it.

@jeddy3
Copy link

jeddy3 commented Nov 19, 2024 •
edited
Loading

Looks ace!

Can I ask if the ambition is to integrate more specific validity cases (not caught by csstree) into this rule or add separate rules for those?

For example, @property requires some descriptors:

@property rules require a syntax and inherits descriptor; if either are missing, the entire rule is invalid and must be ignored. The initial-value descriptor is optional only if the syntax is the universal syntax definition, otherwise the descriptor is required; if it’s missing, the entire rule is invalid and must be ignored.

It'd catch an easy mistake for people to make.

@nzakas
Copy link
Member Author

nzakas commented Nov 19, 2024

@jeddy3

Can I ask if the ambition is to integrate more specific validity cases (not caught by csstree) into this rule or add separate rules for those?

I honestly haven't thought that far ahead yet. 😄 I could imagine wanting that check in this rule, but I'm not 100% sure yet. I'd like to just start with what csstree is providing for the first version and then think about things a bit more once we've got some feedback.

@mdjermanovic
Copy link
Member

There are merge conflicts now. Also, some tests were failing.

mdjermanovic
mdjermanovic reviewed Nov 23, 2024
View reviewed changes
src/util.js Outdated
Comment on lines 10 to 14
/*
* Note: Using `import()` in the JSDoc comments below because including them as
* typedef comments here caused Rollup to remove them. I couldn't figure out why
* this was happening so just working around for now.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be a bug in tree-shaking. When I remove function isSyntaxReferenceError (which is unused and gets removed by tree-shaking either way), Rollup doesn't remove /** @typedef {import("css-tree").SyntaxMatchError} SyntaxMatchError */.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh interesting. Let me look at that.

mdjermanovic
mdjermanovic previously approved these changes Nov 23, 2024
View reviewed changes
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

I'm only not sure if we would want to remove unused function isSyntaxReferenceError, and if this really needs to be merged with the feat! tag since the first version of this package has not been released yet and a similar kind of change #11 was merged with just the feat tag.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdjermanovic mdjermanovic mdjermanovic left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion breaking feature
Projects
Triage
Status: Merge Candidates
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nzakas @jeddy3 @mdjermanovic